Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reimplement hera datasets #2175

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Conversation

peterkrack
Copy link
Contributor

Added rawdata and filter.py script for HERA dataset.
Checks of reimplemented data are still missing.

@giacomomagni giacomomagni linked an issue Oct 16, 2024 that may be closed by this pull request
import yaml

@dataclass
class commondata:
Copy link
Contributor

@giacomomagni giacomomagni Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the filters share similar or even identical parts, you can consider to place it in the filter_utils folder and store common parts there. For ex:

from nnpdf_data.filter_utils.hera import commondata

@scarlehoff
Copy link
Member

hi @peterkrack on top of the checks, make sure to change the process_type to one of these https://github.com/NNPDF/nnpdf/blob/master/validphys2/src/validphys/process_options.py (there should be some DIS types available) and change the kinematic files to use the variables listed there (which to 1st approximation means changing k1, k2, to x,Q or the other way around).

You might need to change Q to Q2 in the kinematic files as well.

@scarlehoff
Copy link
Member

Hi @peterkrack what is the status of this?

@scarlehoff
Copy link
Member

Hi @peterkrack, sorry to bother you again, what is the status of this?

@scarlehoff scarlehoff self-assigned this Nov 13, 2024
@scarlehoff scarlehoff marked this pull request as ready for review November 13, 2024 15:37
@scarlehoff
Copy link
Member

Hi @peterkrack could you include in this PR data-theory comparisons and xq kinematic coverage plots? Thanks

Copy link
Member

@scarlehoff scarlehoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @peterkrack are you available this week to do the changes? Otherwise I'll do them so that we can merge this that has been stale for a few weeks now...

@peterkrack
Copy link
Contributor Author

Plots with k2binsX in NC datasets are not a perfect match yet.

@peterkrack
Copy link
Contributor Author

Setting kinematics_override back to dis_sqrt_scale fixed the issue with the plots.

Copy link
Contributor Author

@peterkrack peterkrack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HERA reimplementation is ready for review.

@peterkrack peterkrack requested a review from t7phy December 12, 2024 12:07
@t7phy
Copy link
Member

t7phy commented Dec 15, 2024

@peterkrack are there any data theory comparisons for all the distributions implemented here? if not, can you please produce a report?

Comment on lines 211 to 221
x:
description: Variable x
label: x
units: ''
k2:
description: Variable k2
label: k2
Q2:
description: Variable Q2
label: Q2
units: ''
k3:
description: Variable k3
label: k3
y:
description: Variable y
label: y
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update the description, label and units here.

@t7phy
Copy link
Member

t7phy commented Dec 16, 2024

Once the above comment is taken care of, this can be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revisit implementation of all DIS
4 participants